Fix columns order in aggregation queries #4
Conversation
django_mongodb/compiler.py
Outdated
| # Reference to a column. | ||
| elif isinstance(expression, int): | ||
| expression = columns[expression] | ||
| selected.append(project_field(expression)) |
There was a problem hiding this comment.
Is there another option where expression isn't str or int and expression is used on the next line as-is?
There was a problem hiding this comment.
🤔 having checked set_values and the method check_alias the fields must be tuples of string. So, I can conclude that everything there is string and the integer are introduced here
django_mongodb/compiler.py
Outdated
| @@ -395,20 +395,10 @@ def build_query(self, columns=None): | |||
| return query | |||
|
|
|||
| def get_columns(self): | |||
| """ | |||
| Return a tuple of (name, expression) with the columns and annotations | |||
There was a problem hiding this comment.
Was the docstring already outdated? You didn't think a new one would be useful?
There was a problem hiding this comment.
I don't think so; the return value remains unchanged. I decided not to modify it because doing so would break other functions, such as subquery.
Long story short: the selection forces changes to the names of some returning columns. This behavior makes it difficult to track when a column's name changes. While I don't know if this was caused by collisions, there's no test to confirm it, and I can't imagine one either.
That said, as long as _make_result returns tuples, we can manage our own names (e.g., __annotation#), and everything should work fine.
So, the docstring is fine.
There was a problem hiding this comment.
Don't know why I deleted it. 😬
There was a problem hiding this comment.
Restored without with period!? 😉
Please check CI failures. I invited you as a collaborator to my fork so hopefully CI will run automatically in the future.
There was a problem hiding this comment.
Great, is everything broken it worked in my computer. 😬 let's fix the things.
|
Does it make sense to send "get_columns as a cached property' to main so the branches don't diverge on that? |
|
It does!
|
timgraham
force-pushed
the
fix-aggregation-queries
branch
from
adc286a to
4716c5e
Compare
timgraham
force-pushed
the
fix-aggregation-queries
branch
from
4716c5e to
67d9d3f
Compare
Adaptations for django/django@65ad4ad.